[Benchmarks] Print all important env vars in debug logs #20415
Conversation
PatKamin
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
PatKamin
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
PatKamin
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Print also PATH and LD_LIBRARY_PATH when these are modified in benchmark commands.
PatKamin
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Fixed |
PatKamin
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
PatKamin
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| for key, value in env_vars.items(): | ||
| old_value = env.get(key, "") | ||
| if old_value: | ||
| env[key] = os.pathsep.join([value, old_value]) |
There was a problem hiding this comment.
what if key on env_vars is not a path, but something existing in env which user wanted to overwrite?
This code is getting more and more complex...
Maybe we can just assume that neither PATH, nor LD_LIBRARY_PATH is not present in env_vars (assert on this) and assume that env_vars overwrite vars and append only PATHS and LD_LIBRARY_PATH from ld_library and add_sycl params.
Sorry that I did not realized this earlier.
There was a problem hiding this comment.
Yeah, this has to be handled well. Unluckily, this assumption is not true, I've checked that. Proposed another solution where there is a neat check and paths are added only to PATH and LD_LIBRARY_PATH. These are the only env vars used in the framework that are lists of paths. All other are just replaced.
| log.debug(f"Running: {full_command_str}") | ||
|
|
||
| # Prepend new value to existing env value | ||
| for key, value in env_vars.items(): |
There was a problem hiding this comment.
now, reading it for the second time I'm wondering.. why do we even have this env_vars? just to print it? If so, why not skip env_vars, set env above, and just print in debug log PATH and LD_LIBRARY_PATH from env...?
Right now, we print something (env_vars), but actually using something else (env).
There was a problem hiding this comment.
I don't want to clutter verbose logs with paths in PATH that are not relevant to benchmarks. It's better to have only relevant paths co I could reproduce easily the env needed to run given commands by hand on the same or other machine.
There was a problem hiding this comment.
yes and no, the other PATHs can actually influence the benchmark... ;)
I still believe my point is valid - we print something (env_vars), but actually using something else for running the command (env).
Anyway, I guess we can live with the currently proposed implementation.
There was a problem hiding this comment.
Yes, I know that the other PATHs could make a difference, but see what is printed now... no PATHs :) From my experience, only these added here are relevant for a quick debug. And that's what I was missing - we printed some env vars, but they were not enough to actually use the printed command and execute it apart from the framework.
|
heh, I think both Łukasz and myself got to the same conclusion - "This code is getting more and more complex..." 😃 |
PatKamin
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
PatKamin
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
PatKamin
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
3 participants
Print also PATH and LD_LIBRARY_PATH when these are modified in benchmark commands.